-
Couldn't load subscription status.
- Fork 4
Add Reactive and Apparent power quantities #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Reactive and Apparent power quantities #23
Conversation
|
@llucax
I think 1) is ok, because the tests seems descriptive and readable now, and I don't see reason to repeat the same function definitions in every file. What do you prefer? |
|
Ideally I would prefer, now what each quantity lives in its own file, to have one test file per quantity, but I think it was very difficult to split the test as it is structured now. I don't think it is worth investing a lot of time on this, at least for now, so based on how complicated you see the split (maybe you could try to ask ChatGPT to do the split, for these types of things I think it can be pretty effective) or just add an ignore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the code is mostly a copy&paste from Power, so not reviewing much in details. I think things get complicated with multiplication and division. From a quick search I find mentions of VAh and VARh as the units for energy coming from apparent and reactive power respectively. I'm not sure if we should introduce also ApparentEnergy and ReactiveEnergy 😬
Maybe @tiyash-basu-frequenz @matthias-wende-frequenz or @cwasicki know better?
|
|
||
| ## Summary | ||
|
|
||
| This is the initial release, extracted from the [SDK v1.0.0rc601](https://github.com/frequenz-floss/frequenz-sdk-python/releases/tag/v1.0.0-rc601). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I wouldn't remove this until 1.0 final.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
At least atm I am not aware of a use-case for this. |
You mean you won't need to get energy from reactive or apparent power? If we don't have uses cases for this and we are not sure what to return when multiplying by time, I would not implement the multiplication by time for these 2 new units for now, we can add them later when we have more clarity, as adding will be a non-breaking change but removing or changing will. |
|
Just saying that I don't know of an app which is using this currently, so adding it later sounds reasonable to me. |
|
577025e to
8f5d96e
Compare
cb25726 to
accb7ed
Compare
Signed-off-by: Elzbieta Kotulska <[email protected]>
Signed-off-by: Elzbieta Kotulska <[email protected]>
Signed-off-by: Elzbieta Kotulska <[email protected]>
This file looks like for now. If number of quantities increase then we should consider separating this file into many quantities. Signed-off-by: Elzbieta Kotulska <[email protected]>
accb7ed to
02974bf
Compare
If necessary, they can be added later. Signed-off-by: Elzbieta Kotulska <[email protected]>
02974bf to
7df70cf
Compare
|
@shsms @ela-kotulska-frequenz do we really need to use I think this is not good, and if we really need this we should think about it more. Let's discuss before a release with these new quantities. |
|
@llucax Please look at last commit in this PR, "Remove the Energy interactions for ReactivePower and ApparentPower" |
|
I am not sure if it s out of scope for this PR/repo, but an explanation or at least a link to what Reactive/Apparent power is in the first place would be nice, no? |
|
@Marenz But if I am wrong, I can write something in follow up PR |
|
Maybe they are well defined, but isn't our target audience not a non-hard-core programmer than can do python stuff and supposedly doesn't have and education in physics? (I never heard of those quantities before I started here for example and actually still don't know what they are). If we expect our target audience to know this, then of course I withdraw my point :) |
|
Documenting active/reactive power is a good thought, but it open up further issues, I feed. Then we need to document every other electrical metric, and we need to document them every time they are used or mentioned. Maybe it is better to look into adding the definition of these terms to our glossary (in a separate issue).
I thought the audience was just novice-programmer. Non-physics-educated sounds new to me. I would have thought that the audience needs to have some knowledge of electricity-related physics, otherwise they would anyway have zero idea of what to do here. |
|
I mean, I was able to string a long pretty far without knowing those two things :P |
Ah, great, sorry for the noise, I must have been looking at an old state of the PR. |
|
About documenting what each quantity is, I think a link to Wikipedia should be enough, no? Very low effort and useful enough for the people that don't know and want to learn. |
No description provided.